Move PolyBFT to Edge #774

Nemanja0x · 2022-10-03T10:35:25Z

Description

Pbft consensus engine together with polybft consensus protocol is introduced with this PR.
Transfer (without transactions processing) polybft including pbft consensus from v3 client to Polygon Edge: https://polygon.atlassian.net/browse/EVM-44

Commands to run

Compile smart contracts:
cd consensus/polybft/polybftcontracts && npm install && npm run compile

Init accounts:
go run main.go polybft-secrets --data-dir test-chain- --num 4
Init genesis:
go run main.go genesis-polybft --block-gas-limit 10000000 --validator-set-size=4
Run nodes:

go run main.go server --data-dir ./test-chain-1 --chain genesis.json --grpc-address :5001 --libp2p :30301 --jsonrpc :10001 --seal --log-level DEBUG
go run main.go server --data-dir ./test-chain-2 --chain genesis.json --grpc-address :5002 --libp2p :30302 --jsonrpc :10002 --seal --log-level DEBUG
go run main.go server --data-dir ./test-chain-3 --chain genesis.json --grpc-address :5003 --libp2p :30303 --jsonrpc :10003 --seal --log-level DEBUG
go run main.go server --data-dir ./test-chain-4 --chain genesis.json --grpc-address :5004 --libp2p :30304 --jsonrpc :10004 --seal --log-level DEBUG

Changes include

Bugfix (non-breaking change that solves an issue)
Hotfix (change that solves an urgent issue, and requires immediate attention)
New feature (non-breaking change that adds functionality)
Breaking change (change that is not backwards-compatible and/or changes current functionality)

Checklist

I have assigned this PR to myself
I have added at least 1 reviewer
I have added the relevant labels
I have updated the official documentation
I have added sufficient documentation in code

Testing

I have tested this code with the official test suite
I have tested this code manually

github-actions · 2022-10-03T10:35:40Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

Stefan-Ethernal · 2022-10-03T10:53:51Z

I have read the CLA Document and I hereby sign the CLA

Nemanja0x · 2022-10-03T11:54:58Z

I have read the CLA Document and I hereby sign the CLA

0xSasaPrsic · 2022-10-03T11:56:50Z

I have read the CLA Document and I hereby sign the CLA

server/builtin.go

consensus/polybft/common/bytes.go

consensus/polybft/proto/messages.proto

command/pbft_genesis/genesis.go

codecov · 2022-10-10T10:25:16Z

Codecov Report

Merging #774 (3392fd1) into feature/v3-parity (d794a2a) will decrease coverage by 5.77%.
The diff coverage is 15.39%.

❗ Current head 3392fd1 differs from pull request most recent head 7998164. Consider uploading reports for the commit 7998164 to get more accurate results

@@                  Coverage Diff                  @@
##           feature/v3-parity     #774      +/-   ##
=====================================================
- Coverage              52.42%   46.64%   -5.78%     
=====================================================
  Files                    132      156      +24     
  Lines                  17478    20472    +2994     
=====================================================
+ Hits                    9162     9549     +387     
- Misses                  7651    10236    +2585     
- Partials                 665      687      +22

Impacted Files	Coverage Δ
consensus/polybft/block_builder.go	`0.00% <0.00%> (ø)`
consensus/polybft/blockchain_wrapper.go	`0.00% <0.00%> (ø)`
consensus/polybft/event_tracker.go	`0.00% <0.00%> (ø)`
consensus/polybft/fsm.go	`0.00% <0.00%> (ø)`
consensus/polybft/hash.go	`0.00% <0.00%> (ø)`
consensus/polybft/polybft.go	`0.00% <0.00%> (ø)`
consensus/polybft/polybft_config.go	`0.00% <0.00%> (ø)`
consensus/polybft/state.go	`0.00% <0.00%> (ø)`
consensus/polybft/state_transaction.go	`0.00% <0.00%> (ø)`
consensus/polybft/system_state.go	`0.00% <0.00%> (ø)`
... and 27 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

zivkovicmilos · 2022-10-12T14:12:50Z

consensus/polybft/polybft.go

+
+	defer newBlockSub.Close()
+
+SYNC:


We shouldn't be using goto directives in Go in 2022, because we couldn't figure out how to write the program flow better.

Please refactor this method flow to not rely on goto label directives.

cc @vcastellm @Kourin1996

cc @ferranbt, wdyt?

We shouldn't be using goto directives in Go in 2022, because we couldn't figure out how to write the program flow better.

I do not think we can generalize on the use of goto. Though I agree that It should not be used as a replacement for control loops, I think there are specific use cases where it is beneficial and we should not automatically discard it because of historical reasons (Note that many other projects from Hashicorp, Kubernetes, and Golang lang itself use goto statements as well).

I would prefer if your concern is about how goto is not being used correctly here and proposals to change the logic instead of an automatic discard.

Anyway, if you feel strongly about this we can bring this topic on the code quality session and add it as part of the code guidelines.

This should be solved case by case, it's still in use yes https://github.com/hashicorp/raft/search?q=goto but agree should be used with care, I expressed the same concern a while ago.

zivkovicmilos · 2022-10-12T14:14:12Z

consensus/polybft/polybft.go

+func (p *Polybft) isSynced() bool {
+	// TODO: Check could we change following condition to this:
+	// p.syncer.GetSyncProgression().CurrentBlock >= p.syncer.GetSyncProgression().HighestBlock
+	syncProgression := p.syncer.GetSyncProgression()
+
+	return syncProgression == nil ||
+		p.blockchain.CurrentHeader().Number >= syncProgression.HighestBlock
+}


I don't think you can rely on sync progression, because the syncing mechanism is always on and always syncing blocks

cc @dbrajovic

This was a requirement for the integration with pbft-consensus. It is expected that the sync protocol is always syncing blocks.

zivkovicmilos · 2022-10-12T14:14:47Z

consensus/polybft/polybft.go

+		select {
+		case <-p.closeCh:
+			return false
+		case <-time.After(2 * time.Second):


How did you come up with this 2s value?

zivkovicmilos · 2022-10-12T14:15:36Z

consensus/polybft/polybft.go

+		numPeers := len(p.config.Network.Peers())
+		if numPeers >= minSyncPeers {
+			break
+		}


This will break single node networks. Please remove this condition, or make minSyncPeers variable

This was made for testing so that we could skip the first Round Change when the network was initializing.

What is the use case of running a single-node network of an instant finally consensus protocol?

zivkovicmilos · 2022-10-12T14:16:18Z

consensus/polybft/polybft.go

+func (p *Polybft) verifyHeaderImpl(parent, header *types.Header, parents []*types.Header) error {
+	blockNumber := header.Number
+	if blockNumber == 0 {
+		// TODO: Remove, this was just for simplicity since I had started the chain already,


zivkovicmilos · 2022-10-12T14:18:30Z

consensus/polybft/polybft.go

+	if !p.isSynced() {
+		// we are currently syncing new data, for sure we are stuck.
+		// We can return 0 here at least for now since that value is only used
+		// for the open telemetry tracing.


Nitpick: Comments should not end with periods

zivkovicmilos · 2022-10-12T14:18:37Z

consensus/polybft/polybft.go

+	}
+
+	// Now, we have to check if the current value of the round 'num' is lower
+	// than our currently synced block.


Nitpick: Comments should not end with periods

zivkovicmilos · 2022-10-12T14:19:00Z

consensus/polybft/polybft.go

+	currentHeader := p.blockchain.CurrentHeader().Number
+	if currentHeader > num {
+		// at this point, it will exit the sync process and start the fsm round again
+		// (or sync a small number of blocks) to start from the correct position.


Nitpick: Comments should not end with periods

consensus/polybft/polybft.go

zivkovicmilos · 2022-10-12T14:32:37Z

consensus/polybft/state.go

+	Sender ethgo.Address
+	// Target is the decoded 'target' field from the event
+	Target ethgo.Address


Please use the types.Address type

zivkovicmilos · 2022-10-12T14:33:13Z

consensus/polybft/state.go

+	// Data is the decoded 'data' field from the event
+	Data []byte
+	// Log contains raw data about smart contract event execution
+	Log *ethgo.Log


Please use the default log type instead of ethgo.Log

We rely on Tracker from the ethgo library for tracking events emitted on the rootchain and it provides us ethgo.Log objects (since it doesn't know for types.Log from the Edge codebase). Therefore unsure whether it is feasible to make such change (I mean we could, but we would need to have conversion from one to another type implemented somewhere and doesn't seem like beneficial, because we couldn't get rid of etgo.Log completely).

Agree with Stefan.

zivkovicmilos · 2022-10-12T14:34:04Z

consensus/polybft/state.go

+	id, ok := raw["id"].(*big.Int)
+	if !ok {
+		return nil, fmt.Errorf("failed to decode id field of log: %+v", log)
+	}
+
+	sender, ok := raw["sender"].(ethgo.Address)
+	if !ok {
+		return nil, fmt.Errorf("failed to decode sender field of log: %+v", log)
+	}
+
+	target, ok := raw["target"].(ethgo.Address)
+	if !ok {
+		return nil, fmt.Errorf("failed to decode target field of log: %+v", log)
+	}
+
+	data, ok := raw["data"].([]byte)


Can you please extract out these map keys to constants?

zivkovicmilos · 2022-10-12T14:37:56Z

consensus/polybft/state.go

+		// paired list
+		keys := make([][]byte, 0)
+		values := make([][]byte, 0)
+		if numberOfSnapshotsToLeaveInDB > 0 { // TODO this is always true?!


zivkovicmilos · 2022-10-12T14:40:17Z

consensus/polybft/state_transaction.go

@@ -0,0 +1,538 @@
+package polybft


Please standardize all the errors in this file, there is no need to use fmt.Errorf everywhere

zivkovicmilos · 2022-10-12T14:42:54Z

consensus/polybft/test_utils.go

@@ -0,0 +1,33 @@
+package polybft


Please rename this file to to include the _test prefix, as it doesn't need to be compiled into the final binary

cc @vcastellm

Fixed through #825

zivkovicmilos · 2022-10-12T14:43:28Z

consensus/polybft/validators_snapshot.go

+	"github.com/hashicorp/go-hclog"
+)
+
+// TODO: Should we switch validators snapshot to Edge implementation? (validators/store/snapshot/snapshot.go)


…lygon/polygon-edge into feature/EVM_44_Move_Polybft_to_Edge

zivkovicmilos · 2022-10-12T14:46:56Z

consensus/polybft/validator_set.go

+func (v validatorSet) CalcProposer(round uint64) pbft.NodeID {
+	var seed uint64
+	if v.last == (types.Address{}) {
+		seed = round
+	} else {
+		offset := 0
+		if indx := v.validators.Index(v.last); indx != -1 {
+			offset = indx
+		}
+
+		seed = uint64(offset) + round + 1
+	}
+
+	pick := seed % uint64(v.validators.Len())
+
+	return pbft.NodeID(v.validators[pick].Address.String())
+}


This method is not very truthful, if validators are simply appended to the end of the validator set. Validators that are added to the end can get the chance to propose the block before some other nodes which were next in line.
Example:

Validators are A, B, C

Block 0 is proposed by A, block 1 by B, block 2 by C

When C is the proposer, a new validator is added to the end (D)

The proposer for block 3 should've been A, but from this method it's D (which is unfair to A)

Validator D should've been the proposer in the next round of proposals, not the current one

This method is not very truthful

As with the validator uptime comment. I think it is hard to come up with a silver bullet strategy for every piece of consensus logic. This in particular is based on the Tendermint proposer calculation which improves some of the security problems in Round Robin for public networks.

zivkovicmilos

I've left extensive comments on this PR because I believe we need to address and discuss some things which can cause major problems in the future. Code style is completely inconsistent. The vast, vast majority of functionality is not covered with tests.

Please resolve the comments, and we can see where to go from there 🙏

Nemanja0x requested review from ferranbt and b00ris October 3, 2022 10:35

github-actions bot added a commit that referenced this pull request Oct 3, 2022

@Stefan-Ethernal has signed the CLA from Pull Request #774

451e279

github-actions bot added a commit that referenced this pull request Oct 3, 2022

@Nemanja0x has signed the CLA from Pull Request #774

606b7c9

github-actions bot added a commit that referenced this pull request Oct 3, 2022

@0xSasaPrsic has signed the CLA from Pull Request #774

86aa9ac

0xSasaPrsic force-pushed the feature/EVM_44_Move_Polybft_to_Edge branch from b921707 to 3166299 Compare October 4, 2022 08:17

ferranbt reviewed Oct 4, 2022

View reviewed changes

server/builtin.go Outdated Show resolved Hide resolved

ferranbt reviewed Oct 4, 2022

View reviewed changes

consensus/polybft/common/bytes.go Outdated Show resolved Hide resolved

ferranbt reviewed Oct 4, 2022

View reviewed changes

consensus/polybft/proto/messages.proto Show resolved Hide resolved

ferranbt reviewed Oct 4, 2022

View reviewed changes

command/pbft_genesis/genesis.go Outdated Show resolved Hide resolved

Stefan-Ethernal added the feature New update to Polygon Edge label Oct 5, 2022

Stefan-Ethernal force-pushed the feature/EVM_44_Move_Polybft_to_Edge branch 3 times, most recently from 81e8324 to 3794de4 Compare October 10, 2022 10:02

Stefan-Ethernal force-pushed the feature/EVM_44_Move_Polybft_to_Edge branch from 3392fd1 to 5f97c5e Compare October 10, 2022 10:29

Nemanja0x and others added 10 commits October 10, 2022 12:39

Initial commit

9f01ed5

some fixes

483d7ed

system_state after rebase

ed4c4d7

remove vendors folder

f5bac88

Add bls signer library.

119ef2f

Event tracker fixed and fms - logger packegs

6ef6667

consensus_runtime fixed

2c466fd

block-builder

362aee9

Remove duplicate method.

b14cf17

Add validators snapshot.

48b179c

zivkovicmilos reviewed Oct 12, 2022

View reviewed changes

consensus/polybft/polybft.go Outdated Show resolved Hide resolved

zivkovicmilos reviewed Oct 12, 2022

View reviewed changes

small change

604c2ff

zivkovicmilos reviewed Oct 12, 2022

View reviewed changes

Victor Castell added 2 commits October 12, 2022 16:43

Move polybft-secrets command to command package

e27dca1

Merge branch 'feature/EVM_44_Move_Polybft_to_Edge' of github.com:0xPo…

7e0e098

…lygon/polygon-edge into feature/EVM_44_Move_Polybft_to_Edge

zivkovicmilos reviewed Oct 12, 2022

View reviewed changes

zivkovicmilos suggested changes Oct 12, 2022

View reviewed changes

Exit early on PolyBFT

7998164

ferranbt approved these changes Oct 12, 2022

View reviewed changes

vcastellm approved these changes Oct 12, 2022

View reviewed changes

goran-ethernal approved these changes Oct 13, 2022

View reviewed changes

ferranbt merged commit c145729 into feature/v3-parity Oct 13, 2022

github-actions bot locked and limited conversation to collaborators Oct 13, 2022

goran-ethernal deleted the feature/EVM_44_Move_Polybft_to_Edge branch October 13, 2022 07:55

goran-ethernal restored the feature/EVM_44_Move_Polybft_to_Edge branch October 13, 2022 08:12

goran-ethernal deleted the feature/EVM_44_Move_Polybft_to_Edge branch October 13, 2022 08:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move PolyBFT to Edge #774

Move PolyBFT to Edge #774

Nemanja0x commented Oct 3, 2022 •

edited by Stefan-Ethernal

Loading

github-actions bot commented Oct 3, 2022 •

edited

Loading

Stefan-Ethernal commented Oct 3, 2022

Nemanja0x commented Oct 3, 2022

0xSasaPrsic commented Oct 3, 2022

codecov bot commented Oct 10, 2022 •

edited

Loading

zivkovicmilos Oct 12, 2022

igorcrevar Oct 12, 2022

ferranbt Oct 13, 2022

ferranbt Oct 13, 2022

vcastellm Oct 13, 2022

zivkovicmilos Oct 12, 2022

ferranbt Oct 13, 2022

zivkovicmilos Oct 12, 2022

zivkovicmilos Oct 12, 2022

ferranbt Oct 13, 2022

zivkovicmilos Oct 12, 2022

zivkovicmilos Oct 12, 2022

zivkovicmilos Oct 12, 2022

zivkovicmilos Oct 12, 2022

zivkovicmilos Oct 12, 2022

zivkovicmilos Oct 12, 2022

Stefan-Ethernal Oct 13, 2022

ferranbt Oct 13, 2022

zivkovicmilos Oct 12, 2022

zivkovicmilos Oct 12, 2022

zivkovicmilos Oct 12, 2022

zivkovicmilos Oct 12, 2022

Stefan-Ethernal Oct 22, 2022

zivkovicmilos Oct 12, 2022

zivkovicmilos Oct 12, 2022

ferranbt Oct 13, 2022

zivkovicmilos left a comment

Move PolyBFT to Edge #774

Move PolyBFT to Edge #774

Conversation

Nemanja0x commented Oct 3, 2022 • edited by Stefan-Ethernal Loading

Description

Commands to run

Changes include

Checklist

Testing

github-actions bot commented Oct 3, 2022 • edited Loading

Stefan-Ethernal commented Oct 3, 2022

Nemanja0x commented Oct 3, 2022

0xSasaPrsic commented Oct 3, 2022

codecov bot commented Oct 10, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zivkovicmilos left a comment

Choose a reason for hiding this comment

Nemanja0x commented Oct 3, 2022 •

edited by Stefan-Ethernal

Loading

github-actions bot commented Oct 3, 2022 •

edited

Loading

codecov bot commented Oct 10, 2022 •

edited

Loading